Implement codemod for replacing unsafe xml methods with defusedxml #76

drdavella · 2023-10-16T20:49:12Z

Overview

Implement codemod for replacing unsafe xml methods with defusedxml

Description

The Python documentation explicitly calls out the provided xml modules as unsafe and recommends the use of defusedxml, which is a third-party package
This codemod replaces the unsafe methods from xml with safe ones from defusedxml
In an effort to break this up into smaller units, this PR only contains the transformation logic itself
I was able to reuse quite a bit of code from the https-connection codemod
NOTE: This codemod is not enabled in the registry yet. See below for additional work.

Next Steps

Add the defusedxml dependency. I am leaving this as separate work because I want to make sure we are adding a good CodeTF comment as well, which requires some additional infrastructure.
Implement an integration test, add documentation, add codemod to the registry

codecov · 2023-10-17T01:17:48Z

Codecov Report

Merging #76 (08735a5) into main (21ae8b2) will increase coverage by 0.12%.
The diff coverage is 96.61%.

@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
+ Coverage   96.21%   96.33%   +0.12%     
==========================================
  Files          46       48       +2     
  Lines        1824     1885      +61     
==========================================
+ Hits         1755     1816      +61     
  Misses         69       69

Files	Coverage Δ
src/codemodder/cli.py	`100.00% <100.00%> (ø)`
src/core_codemods/use_defused_xml.py	`100.00% <100.00%> (ø)`
src/core_codemods/https_connection.py	`97.36% <96.87%> (+3.82%)`	⬆️
src/codemodder/codemods/imported_call_modifier.py	`93.61% <93.61%> (ø)`

src/core_codemods/https_connection.py

src/codemodder/codemods/imported_call_modifier.py

src/core_codemods/use_defused_xml.py

clavedeluna · 2023-10-17T10:50:53Z

tests/codemods/test_use_defused_xml.py

+        """
+
+        new_code = f"""
+            from xml.etree.{module} import ElementPath


this line is unused so I would've expected the remove import visitor to have removed it

That's an interesting point...

will this be something you can resolve now or ticket for later?

I'll ticket it for later; I don't think it directly is a result of this PR.

andrecsilva

My only caveat is the comment about the _matching_function_names. Treat the others as suggestions.

andrecsilva · 2023-10-17T10:26:32Z

src/codemodder/codemods/imported_call_modifier.py

+                # it is a simple name, e.g. call() -> module.new_call()
+                return self.update_simple_name(
+                    true_name, original_node, updated_node, new_args
+                )


Just a small clarification om this bit of code here.
Within the context of https codemod this comment and logic was accurate. But this may not be general enough.
Consider:

from lib import call call()

We have the option to simply change the call...

from lib import call import lib2 lib2.call2()

or changing the call and import:

from lib2 import call2 call2()

The second has an advantage of not having a possibly useless import (not much of a problem since we have a codemod to amend that). Then again, then reason why I've favored the first one for the https codemod was because the change became more explicit.

The second transformation is a bit harder to do but within the realm of possibility. Just tell me if you want me to support that.

@andrecsilva that makes sense. It might be good to revisit at some point but right now I like that the updated call becomes completely explicit.

src/core_codemods/https_connection.py

src/codemodder/codemods/imported_call_modifier.py

andrecsilva · 2023-10-17T16:04:32Z

src/core_codemods/https_connection.py


    METADATA_DEPENDENCIES = (PositionProvider,)

-    matching_functions = {
+    matching_functions: set[str] = {


You could have this be a keys-only dict. It would feel more consistent.

I'm not aware of any such construct; do you just mean a dict where the values are None?

drdavella · 2023-10-17T17:32:59Z

Codecov is upset but I don't think I actually changed the coverage here; I just refactored some previously uncovered lines into a new file that it doesn't recognize.

clavedeluna · 2023-10-17T19:04:40Z

src/codemodder/cli.py

@@ -43,7 +43,7 @@ class CsvListAction(argparse.Action):
    argparse Action to convert "a,b,c" into ["a", "b", "c"]
    """

-    def __call__(self, parser, namespace, values: str, option_string=None):
+    def __call__(self, parser, namespace, values, option_string=None):


I think this was mypy, right? Isn't it weird that we fix a typing problem by ... removing typing?

Yeah I (wrongly) introduced this on a previous PR when mypy wasn't working for me. The underlying code itself is not typed correctly and I thought adding this would fix it (but it does not).

drdavella force-pushed the use-defused-xml branch from 0d0cca6 to f5b024c Compare October 17, 2023 01:09

drdavella force-pushed the use-defused-xml branch from f5b024c to 9180544 Compare October 17, 2023 01:20

drdavella marked this pull request as ready for review October 17, 2023 01:21

drdavella requested review from clavedeluna and andrecsilva as code owners October 17, 2023 01:21

clavedeluna requested changes Oct 17, 2023

View reviewed changes

andrecsilva requested changes Oct 17, 2023

View reviewed changes

drdavella force-pushed the use-defused-xml branch from 9180544 to 747f723 Compare October 17, 2023 15:23

drdavella requested review from andrecsilva and clavedeluna October 17, 2023 15:43

drdavella force-pushed the use-defused-xml branch from 747f723 to cb5c10a Compare October 17, 2023 15:53

andrecsilva approved these changes Oct 17, 2023

View reviewed changes

drdavella force-pushed the use-defused-xml branch from cb5c10a to 4bd530c Compare October 17, 2023 17:07

clavedeluna reviewed Oct 17, 2023

View reviewed changes

drdavella requested a review from clavedeluna October 17, 2023 19:26

clavedeluna approved these changes Oct 17, 2023

View reviewed changes

drdavella added 2 commits October 17, 2023 15:43

Implement codemod for replacing xml with defusedxml

a1a014a

Use simple API base codemod for HTTPS connection

08735a5

drdavella force-pushed the use-defused-xml branch from 4bd530c to 08735a5 Compare October 17, 2023 19:43

drdavella merged commit 57df277 into main Oct 17, 2023
10 of 11 checks passed

drdavella deleted the use-defused-xml branch October 17, 2023 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement codemod for replacing unsafe xml methods with defusedxml #76

Implement codemod for replacing unsafe xml methods with defusedxml #76

drdavella commented Oct 16, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

clavedeluna Oct 17, 2023

drdavella Oct 17, 2023

clavedeluna Oct 17, 2023

drdavella Oct 17, 2023

andrecsilva left a comment

andrecsilva Oct 17, 2023

drdavella Oct 17, 2023

andrecsilva Oct 17, 2023

drdavella Oct 17, 2023

drdavella commented Oct 17, 2023

clavedeluna Oct 17, 2023

drdavella Oct 17, 2023

Implement codemod for replacing unsafe xml methods with defusedxml #76

Implement codemod for replacing unsafe xml methods with defusedxml #76

Conversation

drdavella commented Oct 16, 2023 • edited Loading

Overview

Description

Next Steps

codecov bot commented Oct 17, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrecsilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drdavella commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drdavella commented Oct 16, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading